feat(materials): Inspector preview pane + Material Editor redesign (Phase 5 slice I) - #492

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/MaterialPreviewRenderer_test.cpp (1)

261-426: ⚡ Quick win

Add a thumbnail-after-interactive regression case.

These tests never exercise renderPreview() or renderPreviewAsDataUri() after switching shape or yaw, so the shared-state leak between the interactive and cached preview paths would still pass this suite. One test that renders a cube/plane first and then asserts the thumbnail path returns the default sphere/default-light output would lock that down.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/MaterialPreviewRenderer_test.cpp` around lines 261 - 426, The test suite
misses a regression case where state from renderInteractivePreview leaks into
the cached thumbnail path; add a test in MaterialPreviewRendererTests that first
calls MaterialPreviewRenderer::renderInteractivePreview with a non-sphere shape
(e.g., ShapeCube or ShapePlane) and/or non-zero yaw, then calls the thumbnail
path (MaterialPreviewRenderer::renderPreview or renderPreviewAsDataUri) for the
same material and asserts the thumbnail equals the canonical
sphere/default-light thumbnail (obtained by calling
renderPreview/renderPreviewAsDataUri with ShapeSphere/zero yaw), and ensure both
results are non-empty before comparing; this locks down that
renderInteractivePreview does not contaminate the cached preview state.

qml/PassPropertiesPanel.qml (3)

6-46: 💤 Low value

Local theme color properties duplicated across every panel.

PassPropertiesPanel, TexturePropertiesPanel, and MaterialEditorWindow each re-declare the same eight readonly property color pass-throughs to MaterialEditorQML.*. Consider exposing a tiny ThemePalette.qml singleton (or a single inline component) so the wiring lives in one place and the panels just reference Theme.borderColor / Theme.textColor etc. This isn't blocking, but with the same block duplicated three times in this slice it's already drifting (e.g. some inner GroupBoxes bind directly to MaterialEditorQML.borderColor at lines 69, 203, 406, … while others rely on the root's local alias — see line 22).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/PassPropertiesPanel.qml` around lines 6 - 46, Extract the duplicated
readonly property color pass-throughs into a single shared theme object (e.g.
create a ThemePalette.qml singleton or an inline Component named ThemePalette
exposing borderColor, backgroundColor, panelColor, textColor, highlightColor,
buttonColor, buttonTextColor, disabledTextColor), then remove the eight readonly
property color lines from PassPropertiesPanel and the similar blocks in
TexturePropertiesPanel and MaterialEditorWindow and update their bindings to
reference ThemePalette.borderColor / ThemePalette.textColor / etc. Ensure any
places that currently bind directly to MaterialEditorQML.* are reconciled to use
the new ThemePalette API (or keep MaterialEditorQML delegating to ThemePalette)
so all panels consistently reference the same theme singleton.

---

792-834: ⚡ Quick win

Inline indicator + contentItem overrides cancel the ThemedCheckBox migration.

alphaRejectionEnabledCheck was migrated from CheckBox to ThemedCheckBox, but the entire indicator: Rectangle + contentItem: Text block from the old definition was left in place. The same pattern is duplicated at:

• alphaToCoverageCheck (lines 892-934)
• colourWriteRedCheck / Green / Blue / Alpha (lines 970-1144)
• pointSpritesCheck (lines 1251-1293)
• fogOverrideCheck (lines 1408-1450)

This is ~360 lines of copy-pasted styling that the new themed primitive should encapsulate, and any future styling tweak to ThemedCheckBox will silently not apply to these eight instances. Worth a follow-up pass to strip the overrides and let ThemedCheckBox provide indicator/contentItem (matching the simpler migrations earlier in the same file).

♻️ Example cleanup for `alphaRejectionEnabledCheck`

                 ThemedCheckBox {
                     id: alphaRejectionEnabledCheck
                     text: "Enable Alpha Rejection"
                     checked: MaterialEditorQML.alphaRejectionEnabled
                     onCheckedChanged: {
                         if (checked !== MaterialEditorQML.alphaRejectionEnabled) {
                             MaterialEditorQML.alphaRejectionEnabled = checked
                         }
                     }
                     Connections {
                         target: MaterialEditorQML
                         function onAlphaRejectionEnabledChanged() {
                             alphaRejectionEnabledCheck.checked = MaterialEditorQML.alphaRejectionEnabled
                         }
                     }
-                    indicator: Rectangle {
-                        implicitWidth: 16
-                        ...
-                    }
-                    contentItem: Text {
-                        text: alphaRejectionEnabledCheck.text
-                        ...
-                    }
                 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/PassPropertiesPanel.qml` around lines 792 - 834, ThemedCheckBox instances
still include inline indicator: and contentItem: overrides copied from the old
CheckBox (e.g., alphaRejectionEnabledCheck, alphaToCoverageCheck,
colourWriteRedCheck/Green/Blue/Alpha, pointSpritesCheck, fogOverrideCheck),
which prevents shared styling in ThemedCheckBox from applying; remove the custom
indicator: Rectangle and contentItem: Text blocks from each of these named
controls so they rely on ThemedCheckBox's built-in indicator/contentItem, and if
any custom behavior is required migrate only the minimal differences into
ThemedCheckBox or expose a small property (e.g., accent/spacing) rather than
duplicating full markup.

---

88-146: ⚡ Quick win

Partial ThemedCheckBox migration leaves redundant contentItem overrides.

These ThemedCheckBox instances inline a contentItem: Text { ... } block that re-implements (likely identical) text styling. If ThemedCheckBox is meant to centralize the themed visuals (as the AI summary states), the overrides here defeat the migration: the new primitive's contentItem is replaced by ad-hoc copies that all repeat the same color: textColor / leftPadding: parent.indicator.width + parent.spacing boilerplate. The same pattern repeats below at lines 241-259, 283-301, 325-343, and 367-385.

Drop the per-instance contentItem override and let ThemedCheckBox provide it. If a particular instance needs a tweak, add it as a property on ThemedCheckBox instead of overriding here.

♻️ Suggested simplification (apply to all "Use Vertex Color" + Lighting/Depth toggles)

-                    ThemedCheckBox {
-
-                        text: "Lighting"
-
-                        contentItem: Text {
-
-                            text: parent.text
-
-                            color: textColor
-
-                            leftPadding: parent.indicator.width + parent.spacing
-
-                            verticalAlignment: Text.AlignVCenter
-
-                        }
-
-                        checked: MaterialEditorQML.lightingEnabled
-                        onCheckedChanged: MaterialEditorQML.setLightingEnabled(checked)
-                    }
+                    ThemedCheckBox {
+                        text: "Lighting"
+                        checked: MaterialEditorQML.lightingEnabled
+                        onCheckedChanged: MaterialEditorQML.setLightingEnabled(checked)
+                    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/PassPropertiesPanel.qml` around lines 88 - 146, Several ThemedCheckBox
instances (e.g., the ones bound to MaterialEditorQML.lightingEnabled,
depthWriteEnabled, depthCheckEnabled) unnecessarily override contentItem with
duplicated Text blocks; remove these per-instance contentItem: Text { ... }
overrides and rely on ThemedCheckBox's built-in contentItem styling instead, and
if a specific instance needs a tweak expose a property on ThemedCheckBox (or
pass a simple text/leftPadding property) rather than reimplementing the
contentItem; apply this change to the other repeated occurrences (lines
referenced in the review: the "Use Vertex Color" + Lighting/Depth toggles
blocks) so each checkbox only sets text, checked and onCheckedChanged.

qml/MaterialEditorWindow.qml (2)

532-555: 💤 Low value

Strip console.log debug breadcrumbs before merging.

Component.onCompleted and several MouseArea.onClicked handlers (e.g., lines 235, 277, 318, 361, 500, 1500) log to the console. These are useful during development but become noise in release logs and slow down the inspector when the editor is opened repeatedly. Consider gating them behind a debug flag or removing the ones that no longer serve a purpose.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/MaterialEditorWindow.qml` around lines 532 - 555, Remove or gate
development console logging in Component.onCompleted and in the various
MouseArea.onClicked handlers so debug breadcrumbs don't ship in release builds;
specifically, eliminate or wrap console.log/console.error calls that reference
MaterialEditorQML (seen in Component.onCompleted) and the MouseArea.onClicked
handlers so they either check a central debug flag (e.g., DEBUG or
MaterialEditorDebug) before logging or are removed entirely, preserving
functional calls like MaterialEditorQML.createNewMaterial(...) and the isLoading
state changes.

---

1289-1482: ⚖️ Poor tradeoff

Hidden GroupBoxes kept as binding shims will accumulate technical debt.

The "Techniques", "Passes", and "Texture Units" GroupBoxes (lines 1298-1350, 1354-1409, 1420-1474) are now visible: false but still construct themselves, instantiate ThemedComboBox + ThemedButton, and wire change handlers (e.g., techniqueCombo.onCurrentIndexChanged: MaterialEditorQML.setSelectedTechniqueIndex(currentIndex)). They also lack the sentinel-aware behavior of the new sticky context strip, so if anything ever causes them to be re-shown they will silently fight the context-strip combos for the same backend selection.

The comments mention they're kept "for binding stability" / "existing tests / external tools" — once the slice is merged, please plan a follow-up to either remove them outright or extract the binding shim into a non-visual Item so dead GroupBox chrome doesn't keep being rendered/restyled in every subsequent theming pass.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/MaterialEditorWindow.qml` around lines 1289 - 1482, Hidden GroupBoxes
(Techniques, Passes, Texture Units) are still instantiating UI chrome and
controls; replace each GroupBox with a non-visual binding shim Item (e.g.,
TechniqueBindingShim, PassBindingShim, TextureUnitBindingShim) that contains
only the ThemedComboBox/ThemedButton IDs (techniqueCombo, passCombo, textureUnit
combo IDs) and their onCurrentIndexChanged handlers
(MaterialEditorQML.setSelectedTechniqueIndex/setSelectedPassIndex/setSelectedTextureUnitIndex)
and newXDialog references, so bindings remain but no GroupBox/Rectangle/label
styling is created; keep Layout.fillWidth/disabled logic if needed but ensure
the shim is non-visual (no background/label) to avoid extra styling passes and
potential conflicts with the sticky context strip.

src/PropertiesPanelController_test.cpp (1)

991-1070: 💤 Low value

Good coverage for the new applyMaterialToSelection behavior.

The four new tests cleanly exercise: empty-name no-op, no-selection return value, sub-entity precedence over parent entity, full-entity fan-out across sub-entities, and the selectionChanged emission contract. The Xvfb gate via canLoadMeshFiles() is correctly placed before every test that creates an entity, matching the existing pattern in this fixture.

A couple of tiny opportunities (not blocking):

• createStandardOgreMaterials() is already called once in SetUp() (line 42); the re-invocations at lines 1015, 1037, and 1057 are redundant — fine for idempotency but easy to drop.
• sub0->getMaterial()->getName() and entity->getSubEntity(0)->getMaterial()->getName() will crash if the material pointer is ever null. Today the prior EXPECT_EQ(..., 1) makes that unreachable, but an ASSERT_TRUE(sub0->getMaterial()) (or ASSERT_FALSE(...->isNull()) for MaterialPtr) before dereferencing would give a clean test failure instead of a segfault if applyMaterialToSelection regresses in the future.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/PropertiesPanelController_test.cpp` around lines 991 - 1070, Remove the
redundant createStandardOgreMaterials() calls in the tests
ApplyMaterialToSelection_SubEntityTakesPrecedence,
ApplyMaterialToSelection_EntitySelectionTouchesAllSubmeshes and
ApplyMaterialToSelection_EmitsSelectionChangedSignal (it's already called in
SetUp via createStandardOgreMaterials()); and add assertions to guard
dereferencing of material pointers before calling getName(), e.g. in
ApplyMaterialToSelection_SubEntityTakesPrecedence assert that
sub0->getMaterial() is non-null (ASSERT_TRUE(sub0->getMaterial()) or
ASSERT_FALSE(sub0->getMaterial().isNull())) and similarly assert
entity->getSubEntity(0)->getMaterial() is non-null in tests that inspect that
material to avoid segfaults if applyMaterialToSelection regresses.

qml/PropertiesPanel.qml (1)

1819-1930: ⚡ Quick win

Remove ScrollView wrapping and attach ScrollBar.vertical directly to the GridView.

GridView is a Flickable, and wrapping it in ScrollView creates a nested-Flickable issue that can cause gesture handling conflicts and scrollbar synchronization problems. The Qt 6 documentation and the established pattern used elsewhere in this codebase (e.g., AISettingsDialog.qml, AssetBrowser.qml) show that ScrollBar.vertical should be attached directly to the grid instead.

♻️ Suggested restructure

-                ScrollView {
-                    anchors.fill: parent
-                    anchors.margins: 1
-                    clip: true
-                    visible: materialToolCol.gridReady
-
-                    GridView {
-                        id: matGrid
-                        cellWidth: Math.max(90,
-                            (width - 4) / Math.max(1, Math.floor((width - 4) / 90)))
-                        cellHeight: 100
-                        model: materialToolCol.materialNames
-                        delegate: Item {
-                            ...
-                        }
-                    }
-                }
+                GridView {
+                    id: matGrid
+                    anchors.fill: parent
+                    anchors.margins: 1
+                    clip: true
+                    visible: materialToolCol.gridReady
+                    cellWidth: Math.max(90,
+                        (width - 4) / Math.max(1, Math.floor((width - 4) / 90)))
+                    cellHeight: 100
+                    model: materialToolCol.materialNames
+                    ScrollBar.vertical: ScrollBar {}
+                    delegate: Item {
+                        ...
+                    }
+                }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@qml/PropertiesPanel.qml` around lines 1819 - 1930, The ScrollView wrapper
creates a nested Flickable problem; remove the ScrollView and make the GridView
(id: matGrid) the direct child of the Rectangle, set its anchors.fill (and
anchors.margins: 1) and clip: true, keep its visible bound to
materialToolCol.gridReady, and then attach a ScrollBar.vertical to matGrid
(using ScrollBar.vertical: ScrollBar { } or ScrollBar { id: matGridVScroll;
orientation: Qt.Vertical; /* styling */ } declared as the GridView's
ScrollBar.vertical) so the flicking and scrollbar are handled by the GridView
itself; ensure the loading Text remains visible when !materialToolCol.gridReady
and MouseArea/delegate behavior is unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes